refactor(node-resolve): remove deep-freeze and deepmerge from dependencies #529
Conversation
packages/node-resolve/src/index.js
Outdated
| const defaults = { | ||
| customResolveOptions: {}, | ||
| dedupe: [], | ||
| const defaults = Object.freeze({ |
There was a problem hiding this comment.
I'd rather see a simple convenience method (const deepFreeze = () =>) in here to take the place of the removed dep on line 30, rather than a buncha calls to Object.freeze that we'd have to remember to add to anything new to defaults. A depth of 4 seems like it would be sufficient. The code is rather simple.
|
Isn't it much easier to just duplicate defaults definition twice than adding a bunch of packages for very doubtful use case (laziness)? |
|
I was actually considering copy and pasting the defaults. @shellscape, what do you think? |
man, I really take exception to that language. @alan-agius4 I'd ask that you follow the guidance offered in my review |
|
By laziness I mean initial issue with adding ts extensions. It's really the only use case and requires two dependencies though it's not a big deal to list all necessary extensions in user config. |
alan-agius4
force-pushed
the
node-resolve-deps
branch
3 times, most recently
from
9076f9e to
c467634
Compare
Sorry guys, not backing down from that. |
|
@shellscape, I added the deep freeze implementation. |
|
DeepFreeze package seems to be unmaintained. They do have a PR (https://github.com/substack/deep-freeze/pull/20 ) to change the license type but is staled. |
|
Can you explain what do you mean by deepmerge being a guard? |
|
Dude, please use the edit feature rather than three consecutive replies. No one likes their notifications to go bananas. It's a guard to prevent mutating the exported defaults. Because this is compiled down to CJS, defaults could inadvertently be overwritten, causing program failure. Providing what is essentially a deep That code was put in place precisely because someone did exactly what is described above and encountered errors. Please assume that we're not sitting around coming up with ways to be difficult. If a maintainer says it's a hard block and they aren't backing down, you can safely assume there's a good reason. |
|
@shellscape, apologies for the extra notifications. I believe that If it makes you happy and we can merge this PR I can add back deepmerge, but again it seem redundant to me considering you are freezing the object itself and all of it’s sub properties. |
`deep-freeze` is licensed as Public Domain, which might be problematic for some 3rd parties such as the Angular CLI. In Angular CLI we have a license validator that validates direct and transitive dependencies, and Public Domain is a problematic license becuse it falls under the "unencumbered' group which requires legal audit. More context: https://opensource.google/docs/thirdparty/licenses/#unencumbered
alan-agius4
force-pushed
the
node-resolve-deps
branch
from
c467634 to
54a3a52
Compare
Unfortunately that's not true. This can be simply demonstrated. // a.js
module.exports = { defaults: Object.freeze({ batman: 'batman' }) };→ node
Welcome to Node.js v12.18.1.
Type ".help" for more information.
> require('./a').defaults
{ batman: 'batman' }
> require('./a').defaults = null
null
> require('./a').defaults
null |
|
@shellscape, I see what you mean now. Thanks for the explanation. But honestly if someone is doing that, they should be punished and not handle such cases. I cannot think of a genuine use case that it would be okay to that. Also possible in this case a top level Ps: deepmerge re-implemented |
Thankfully, we have empathy for users in this project. Change looks good. I'm on my phone right now and will merge when I return home. |
|
@alan-agius4 next time please use our Pull Request template. normally a script I run on my machine would have picked up that up and closed the PR citing the missing template. Guess you got lucky :) |
) `deep-freeze` is licensed as Public Domain, which might be problematic for some 3rd parties such as the Angular CLI. In Angular CLI we have a license validator that validates direct and transitive dependencies, and Public Domain is a problematic license becuse it falls under the "unencumbered' group which requires legal audit. More context: https://opensource.google/docs/thirdparty/licenses/#unencumbered
This 2 dependencies are currently used to deepfreeze an exported symbol. Which seems to be an wasteful to add 2 extra dependencies for something trivial.
Also
deep-freezeis licensed as Public Domain, which might be problematic for some 3rd parties such as the Angular CLI.In Angular CLI we have a license validator that validates direct and transitive dependencies, and Public Domain is a problematic license becuse it falls under the "unencumbered' group which requires legal audit.
More context: https://opensource.google/docs/thirdparty/licenses/#unencumbered